Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Jump to bottom

Fix redundant imports when selecting options from the dropdown widget #7028

Merged
merged 8 commits into from
Jun 21, 2023
Merged

Fix redundant imports when selecting options from the dropdown widget #7028

merged 8 commits into from
Jun 21, 2023

Conversation

vitvakatu
Copy link
Contributor

@vitvakatu vitvakatu commented Jun 14, 2023 •
edited
Loading

Pull Request Description

Fixes #6816.

The code for adding imports for dropdown widgets was unified with CB. The code was moved from the searcher controller to the graph controller.

Also, I changed the signature for a few lookup_* methods of the suggestion database, because I have always found it weird that they return Option instead of Result. They now work nicely with the surrounding code.

2023-06-15.17-57-13.mp4

Important Notes

Checklist

Please ensure that the following checklist has been satisfied before submitting the PR:

  • The documentation has been updated, if necessary.
  • Screenshots/screencasts have been attached, if there are any visual changes. For interactive or animated visual changes, a screencast is preferred.
  • All code follows the
    Scala,
    Java,
    and
    Rust
    style guides. In case you are using a language not listed above, follow the Rust style guide.
  • All code has been tested:
    • Unit tests have been written where possible.
    • If GUI codebase was changed, the GUI was tested when built using ./run ide build.

@vitvakatu vitvakatu self-assigned this Jun 14, 2023
@vitvakatu vitvakatu force-pushed the wip/vitvakatu/redundant-imports branch from 0c2e6df to 8e65ae7 Compare June 15, 2023 13:51
@vitvakatu vitvakatu marked this pull request as ready for review June 15, 2023 14:00
kazcw
kazcw approved these changes Jun 15, 2023
View reviewed changes
Copy link
Contributor

@kazcw kazcw left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Looks good. A few nits.

app/gui/src/controller/graph.rs Outdated Show resolved Hide resolved
app/gui/src/controller/graph.rs Outdated Show resolved Hide resolved
app/gui/src/controller/graph.rs Outdated Show resolved Hide resolved
@xvcgreg
Copy link

xvcgreg commented Jun 19, 2023

@vitvakatu What's the status here?

wdanilo
wdanilo approved these changes Jun 19, 2023
View reviewed changes
Copy link
Member

@wdanilo wdanilo left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Nice solution!

@vitvakatu
Copy link
Contributor Author

@xvcgreg QA requested, @Frizi will report results.

@Frizi
Copy link
Contributor

Frizi commented Jun 20, 2023

QA Status: 💚

Looks great, barely anything now gets imported when from Standard.Base import all is present. And when I manually remove that import, the appropriate gets correctly imported on demand.

@vitvakatu vitvakatu added the CI: Ready to merge This PR is eligible for automatic merge label Jun 20, 2023
@mergify mergify bot merged commit 01a5361 into develop Jun 21, 2023
@mergify mergify bot deleted the wip/vitvakatu/redundant-imports branch June 21, 2023 12:00
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@Frizi Frizi Frizi left review comments

@wdanilo wdanilo wdanilo approved these changes

@kazcw kazcw kazcw approved these changes

@MichaelMauderer MichaelMauderer Awaiting requested review from MichaelMauderer

@farmaazon farmaazon Awaiting requested review from farmaazon farmaazon is a code owner

@mwu-tow mwu-tow Awaiting requested review from mwu-tow

Assignees

@vitvakatu vitvakatu

Labels
CI: Ready to merge This PR is eligible for automatic merge
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.

IDE inserts import for Boolean when True is selected from the dropdown
5 participants
@vitvakatu @xvcgreg @Frizi @kazcw @wdanilo